Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement DepTrackingHash for Option through blanket impls instead of macros #84234

Merged
merged 1 commit into from
Jun 5, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Apr 15, 2021

This avoids having to add a new macro call for both the Option and the type itself.

Noticed this while working on #84233.
r? @Aaron1011

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 15, 2021
@jyn514 jyn514 added A-incr-comp Area: Incremental compilation T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 15, 2021
@Aaron1011
Copy link
Member

Aaron1011 commented Apr 16, 2021

Somewhat unrelated to this PR - I think defining the order-independence of the hash at the type level is the wrong way to go about this. Nothing is preventing the rest of the compiler from relying on the option order.

Instead, I think DepTrackingHash shouldn't do any re-ordering at all. Order-independence can be implemented by having the option parsing code sort the parsed Vec immediately after it gets parsed. That makes it impossible for any compiler code to accidentally rely on the original command-line argument order.

@jyn514
Copy link
Member Author

jyn514 commented Apr 22, 2021

So I'm looking back over this - I think the only way this could change a hash is that Option<Vec<String>> will no longer be sorted before being hashed. That's looks like it's only used for allow_features, which shouldn't need to depend on the order:

$ rg ': Option<Vec<String.*\[TRACKED' compiler/rustc_session/src/options.rs
895:    allow_features: Option<Vec<String>> = (None, parse_opt_comma_list, [TRACKED],

Having Option<Vec> be unsorted but Vec be sorted seems really fishy to me, I think changing this is ok.

@jyn514
Copy link
Member Author

jyn514 commented Apr 22, 2021

Instead, I think DepTrackingHash shouldn't do any re-ordering at all. Order-independence can be implemented by having the option parsing code sort the parsed Vec immediately after it gets parsed. That makes it impossible for any compiler code to accidentally rely on the original command-line argument order.

That makes sense to me. I see the following Vecs used:

$ rg ': .*Vec<.*\[TRACKED' compiler/rustc_session/src/options.rs
86:        crate_types: Vec<CrateType> [TRACKED],
92:        lint_opts: Vec<(String, lint::Level)> [TRACKED],
97:        libs: Vec<(String, Option<String>, NativeLibKind)> [TRACKED],
829:    llvm_args: Vec<String> = (Vec::new(), parse_list, [TRACKED],
833:    metadata: Vec<String> = (Vec::new(), parse_list, [TRACKED],
851:    passes: Vec<String> = (Vec::new(), parse_list, [TRACKED],
895:    allow_features: Option<Vec<String>> = (None, parse_opt_comma_list, [TRACKED],
920:    crate_attr: Vec<String> = (Vec::new(), parse_string_push, [TRACKED],

Are llvm's arguments order-sensitive? Or passes? The others look like they should be order insensitive.

@jyn514
Copy link
Member Author

jyn514 commented Apr 22, 2021

Ok, I take it back, I see the following order-dependent uses:

compiler/rustc_save_analysis/src/lib.rs
97:        let crate_type = sess.crate_types()[0];

and indirectly through dependency_formats (which depends on the order of crate_types):

compiler/rustc_codegen_ssa/src/back/linker.rs
1253:    let formats = tcx.dependency_formats(LOCAL_CRATE);
1254-    let deps = formats.iter().find_map(|(t, list)| (*t == crate_type).then_some(list)).unwrap();

There might be more, I only looked at crate_types.

Should I just sort all these and hope nothing breaks?

@bors

This comment has been minimized.

@Aaron1011
Copy link
Member

Aaron1011 commented May 1, 2021

Should I just sort all these and hope nothing breaks?

The line in rustc_save_analysis seems questionable - I don't think we have a notion of 'primary/main crate type' anywhere else.

I'm not sure about dependency_formats - I'd recommend asking someone more familiar with that code. For now, I think it would be fine to leave that unsorted, and investigate further if anyone complains about the performance impact of switching around --crate-type flags during incremental compilation.

@Aaron1011 Aaron1011 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 1, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 4, 2021
…nkov

Reduce duplication in `impl_dep_tracking_hash` macros

Cherry-picked from rust-lang#84234 since it will be a while until it lands.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 4, 2021
…nkov

Reduce duplication in `impl_dep_tracking_hash` macros

Cherry-picked from rust-lang#84234 since it will be a while until it lands.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 4, 2021
…nkov

Reduce duplication in `impl_dep_tracking_hash` macros

Cherry-picked from rust-lang#84234 since it will be a while until it lands.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 4, 2021
…nkov

Reduce duplication in `impl_dep_tracking_hash` macros

Cherry-picked from rust-lang#84234 since it will be a while until it lands.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 4, 2021
…nkov

Reduce duplication in `impl_dep_tracking_hash` macros

Cherry-picked from rust-lang#84234 since it will be a while until it lands.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request May 4, 2021
…nkov

Reduce duplication in `impl_dep_tracking_hash` macros

Cherry-picked from rust-lang#84234 since it will be a while until it lands.
@jyn514
Copy link
Member Author

jyn514 commented May 5, 2021

I won't have time to follow up on this in the next month or so. Do you think this is ok to land as-is or do you want the broader change with sorting first?

RalfJung added a commit to RalfJung/rust that referenced this pull request May 5, 2021
…nkov

Reduce duplication in `impl_dep_tracking_hash` macros

Cherry-picked from rust-lang#84234 since it will be a while until it lands.
RalfJung added a commit to RalfJung/rust that referenced this pull request May 5, 2021
…nkov

Reduce duplication in `impl_dep_tracking_hash` macros

Cherry-picked from rust-lang#84234 since it will be a while until it lands.
RalfJung added a commit to RalfJung/rust that referenced this pull request May 5, 2021
…nkov

Reduce duplication in `impl_dep_tracking_hash` macros

Cherry-picked from rust-lang#84234 since it will be a while until it lands.
@bors

This comment has been minimized.

@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 19, 2021
@bors

This comment has been minimized.

@Aaron1011 Aaron1011 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 1, 2021
…of macros

This avoids having to add a new macro call for both the `Option` and the
type itself.
@jyn514 jyn514 changed the title Implement DepTrackingHash through blanket impls instead of macros Implement DepTrackingHash for Option through blanket impls instead of macros Jun 4, 2021
@jyn514 jyn514 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 4, 2021
@jyn514
Copy link
Member Author

jyn514 commented Jun 4, 2021

Ok, this is a nice small PR now.

@Aaron1011
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jun 4, 2021

📌 Commit 76502de has been approved by Aaron1011

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2021
@bors
Copy link
Contributor

bors commented Jun 5, 2021

⌛ Testing commit 76502de with merge 9e6f0e8...

@bors
Copy link
Contributor

bors commented Jun 5, 2021

☀️ Test successful - checks-actions
Approved by: Aaron1011
Pushing 9e6f0e8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 5, 2021
@bors bors merged commit 9e6f0e8 into rust-lang:master Jun 5, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 5, 2021
@jyn514 jyn514 deleted the blanket-hash branch June 5, 2021 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-incr-comp Area: Incremental compilation merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants